Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC for bundler checksum verification #50

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

martinemde
Copy link
Member

This is a draft based on the feature work in rubygems/rubygems#6374.

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typography comment.

text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved
text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved
text/0011-gem-checksum-verification.md Show resolved Hide resolved
Delete the downloaded gem files referenced above and run `bundle install`

If you are sure that the downloaded gem is correct, you can run…
(RFC TODO: Currently undecided on what the command should be here to allow unverified gems.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... musing on this a bit, should we instead allow overriding a specific sha in the gemfile? that way, users have a way to still securely pin to a checksum, and only override the single gem, rather than blanket disabling verification

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think gem "foo", checksum_override: "abc123" is probably the most reasonable way to do this... if we actually want to allow it? I'm not sure if we do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would have to be a hash, because each platform would have its own checksum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error could output the command to run to ignore this specific gem's mismatch and then we would just record it in the gemfile.lock. I imagine just changing the relevant line in the CHECKSUMS section to rake (1.0.0) IGNORED. Then since it's transiently in the lockfile, the next update to a different version would either produce a new error or resolve correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we entirely remove the message about disabling the feature and just update the docs?

This would be in addition to adding a command for ignoring a single checksum mismatch, if we decide to do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the use case for needing this? Could we instead allow users to vendor the specific gem with this checksum? Having random hardcoded checksums in the Gemfile feels weird. Everybody will need to have access to the source .gem for it to work, right? So how come would the mismatch happen?

Maybe alternatively (if I understand correctly how this feature would play with vendoring) we could allow users to vendor specific .gem files? I think this would be similar to what was requested at rubygems/rubygems#4495?

text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved
text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved

Platforms may pose a problem for Gemfile.lock checksums. If a user has not added one of the platforms where they bundle their app, then checksum verification could be skipped. In order to avoid this security vulnerability, the following should happen automatically.

1. When a gem is installed and no checksum is available in the lockfile, a warning should be printed indicating that unverified gems were installed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be printed even if there's a checksum coming from the remote source? if so, that feels like it would get annoying

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a conflict between storing the checksums from the .gem on install and not installing unverified gems in production when the platform is missing. I think as you commented below, we want to be transparent about this unless the bundle is frozen.

I think we need to add this feature then: If new checksums are recorded during install while the bundle is frozen, then fail.

Copy link
Member Author

@martinemde martinemde Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let me refine what I said there, because I don't think it captures the whole picture and I think there are conflicts.

Let's just talk about the frozen bundle situation. Without frozen, installs will never be blocked unless a checksum mismatches.

Here's the bundler docs about frozen:

--frozen
Do not allow the Gemfile.lock to be updated after this install. Exits non-zero if there are going to be changes to the Gemfile.lock.
This option is deprecated in favor of the frozen setting.

Given the lock (and assuming these gems are all added to the bundle already, and would install normally before 2.5.0):

CHECKSUMS
  foo (1.0.0)
  bar (1.0.0-darwin-23)

When does installing the gem fail during a frozen bundle?

  1. Install foo. A checksum is computed at install and it would be recorded.
  2. Install bar (1.0.0-x86_64-linux) which is entirely missing from the CHECKSUMS section.
  3. Install baz which is entirely missing from the CHECKSUMS section.
  4. All of the above because if you're running in frozen mode, the docs say it will fail if any update would produce a change to the lock.

This is a real challenge because this is going to happen to a lot of people:

  1. Update bundler version
  2. Run bundle install (no checksums are written unless the gems are not already installed)
  3. Commit and push to production
  4. Frozen install fails because new checksums are found.

This means we need to make exceptions for checksums being found from gem installs even when frozen. It could print a warning saying that we ignored missing checksums. (we also then need to distinguish between missing and ignored checksums)

Or we need to say "if you're running in frozen mode, all checksums must be recorded". This means running bundle lock before you push and might require that we inform people when we first write the CHECKSUMS section that they need to run lock.

After people are aware of this constraint, they would expect us not to install a gem without checksums during a frozen install. Imposing this constraint immediately is just going to cause a bunch of deployment failures.

Copy link
Member

@deivid-rodriguez deivid-rodriguez Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be fixed in we started recording checksums only for fresh lockfiles? That means people with existing lockfiles (and thus everyone using frozen) would not be affected initially. We could also add a bundle lock --add-checksums flag for people to opt-in to the feature on an existing lockfile.

text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved

### How do we handle confusion about the authority of checksums written to the Gemfile.lock

The source of checksums in the Gemfile.lock becomes a matter of trust once it's written. Did the checksum come from the API or was it calculated from a .gem file on a developers computer? If a checksum error is resolved by one developer in a way that saves an incorrect checksum, how should people know when to approve these changes or not? It may not even be common practice for most teams to look at the Gemfile.lock, and changes can often be hidden in pull request reviews. Without a process for checking that the checksums are trustworthy, it's left to every development team to decide on a process. One solution would be a bundle command that could be run in CI every time the gems are installed that verifies the authenticity of checksums in the Gemfile.lock.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would look to other package managers here and see what they do (which in my experience is usually strictly additive?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might imagine writing a step in CI that runs bundle lock --frozen (and I'm making this command up) and the goal would be that it fetches the checksums, but generates an error if anything would be written to the Gemfile.lock.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally found some time to review this. This is so great @martinemde, impressive! Fully onboard with your careful thorough approach!


If you wish to immediately add checksums to your lockfile for all locked gems, run `bundle lock`.
Bundle lock now fetches checksums from remote sources by default.
If you would like to bypass this behavior, add the `--no-checksums` flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if I understand correctly. The only way to opt-into checksums will be to run bundle lock explictly?

In other words, will bundle install record checksums to the Gemfile.lock when it's actually generating it for the first time? I think we'll want to do eventually do this, but it probably doesn't hurt to battle test the feature fist by explicitly opting in.

Since users need to explicitly opt-in to this, would it make sense to do it under a --add-checksums flag, to make sure there's no change to existing behavior?

Copy link
Member Author

@martinemde martinemde Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is enabled by default and every install or update will save and compare checksums.

However, for most people they are not performing a clean install. If you run bundle install and nothing is installed from gem, you get no checksums. I wanted to provide a way for people to explicitly dive in fully.

I think bundle lock should just always add all available checksums from all sources to the lock.

The full explicit record and test currently works via bundle lock --update --bundler (to avoid gem changes) and then bundle pristine which compares every gem to the recorded checksum. I want lock to grab checksums without the extra args.

If you would like to bypass this behavior, add the `--no-checksums` flag.

Common Bundler commands like `bundle install`, `bundle update`, `bundle lock`, `bundle add` will now automatically record and verify checksums.
Commands that rely on checksums for verification will output a message or warning when checksums are missing or mismatched.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should also be a hard failure in frozen mode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see an argument either way, but it's in my mind a bit of a stretch of what frozen already means (for missing checksums, at least).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the missing case, I meant only for GEM sources (which if I understood correctly are the only ones adding checksum). And also only if the Gemfile is already using CHECKSUMS.

Copy link
Member Author

@martinemde martinemde Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bundler docs:

Do not allow the Gemfile.lock to be updated after this install. Exits non-zero if there are going to be changes to the Gemfile.lock.

frozen (BUNDLE_FROZEN): Disallow changes to the Gemfile. When the Gemfile is changed and the lockfile has not been updated, running Bundler commands will be blocked. Defaults to true when --deployment is used.

I think there's 2 ways we could interpret this:

  1. The first quote would suggest that we cannot make any change to the Gemfile.lock, so a frozen bundle should fail if checksums are missing. People use frozen for added security (don't install something different than what was committed and tested) and this fits with added security. If your production environment wants to install the alpine linux version of a bundled gem and you don't have a checksum for it then hard fail.

  2. The second quote seems to say that if there are changes to the Gemfile not reflected in the Gemfile.lock, then fail. A missing checksum for a bundle that would otherwise work in frozen mode is not a clear violation of that rule. With this approach, bundler stays exactly as secure as it was previously in this case. Installing the alpine linux version of a gem without being able to verify the checksum is ok.

Option 1 seems implied to me, but option 2 is fully within existing expectations and will break less CI builds. The question is: should we break those CI builds? Is that what those engineers expected bundler to do? If frozen doesn't break that build, then what will?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My interpretation of frozen has always been 1. For example, if we run under a new platform that's not already in the lockfile, we fail in frozen mode even if there's no Gemfile changes. Basically in frozen mode, everything should be taken from the lockfile.

In addition to this, I think this feature kind of misses the point if we don't fail when we find a mismatch and instead happily install the mismatched gem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If frozen doesn't break that build, then what will?

Exactly.

text/0011-gem-checksum-verification.md Outdated Show resolved Hide resolved
text/0011-gem-checksum-verification.md Show resolved Hide resolved
2. run `bundle install`

To ignore checksum security warnings, disable checksum validation with
`bundle config set --local disable_checksum_validation true`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this setting also control whether checksums are added to the lockfile or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO no, if we want a way to avoid putting checksums in the lock file (which im also not sure we want), it should be under a different flag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure we want it, but I reckon it could be useful for a graceful migration, given that the setting already exists, even if we'll eventually remove the flag.

It'd be interesting to check why we added this flag in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It was there when I accidentally stole this work from @segiddins. It's a failsafe in case we really mess someone up or they like using hacked and corrupted gems and wish to continue doing so.

text/0011-gem-checksum-verification.md Show resolved Hide resolved
text/0011-gem-checksum-verification.md Show resolved Hide resolved
When gems come from private gem servers that do not implement the compact index, checksums will not be available.
Bundler will calculate digests from .gem files from sources that don’t supply checksums.

An additional flag for `bundle lock` could be provided that allows reinstalling, and thus calculating the checksum for, gems that don't have a checksum.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the behavior for remote servers will be that there's only one level of verification, i.e., that the checksum of whatever gem that was once installed is recored in the lockfile and should be the same in for future installs.

What happens for bundle lock without the additional flag? Does it record empty checksums? Should it download gems to cache and compute their checksums without installing anything? Maybe too slow? I guess a mismatch " != " will be ignored and result in the non empty checksum being locked?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the behavior for remote servers will be that there's only one level of verification, i.e., that the checksum of whatever gem that was once installed is recored in the lockfile and should be the same in for future installs.

it will also be checked against what the server reports in the compact index, if that is present (which is what already happens)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant "for remote servers that don't implement the compact index"!

Copy link
Member Author

@martinemde martinemde Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On first install we will compute, compare and store all checksums for all .gem files that are downloaded. If you run bundle pristine get every checksum from every gem that can be installed from .gem source.

As for an explicit way to fill in missing gems besides that, I don't know.

I'm actually concerned about another similar case, a broken gem that someone depends on and needs. Theoretically the server sends a bad checksum and the gem continually clashes. Can you ignore just that gem?

Also, missing checksums that keep breaking frozen bundles, like for different platforms or that didn't get recorded. Frozen says "raises if lock would be changed."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand that case very well. What do you mean by "a broken gem"? Maybe a gem with custom patches or something? Even if they are doing that, shouldn't they upload it to some gem server so they can distribute it to all users of the lockfile? Wouldn't all users of the lockfile want the exact same version?

Regarding missing platforms, I plan to lock all platform soon, so that shouldn't be a problem I think.

Lockfiles having only ruby platform are a problem because they don't lock specific platform gems. Instead, they pick the most suitable platform specific version at install time. So checksums won't play nice with these lockfiles I believe. And they are still the majority.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. Anything broken like that should be a fail and we should address the failure from it, not preemptively prepare for it.

So checksums won't play nice with these lockfiles I believe. And they are still the majority.

This seems like the distinction between frozen (strict) and unfrozen. If you want to use bundler loosely, then you also won't get this extra assurance from checksums.

Copy link
Member

@deivid-rodriguez deivid-rodriguez Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, if a lockfile is using this loose mode, then you don't want checksums.

If you do explicitly add checksums to this kind of lockfile, that means you opt-in to strict mode, and you'll only get gems without specific platforms locked in there, with their respective checksums, and that's what will be consistently installed, regardless of the platform where the lockfile is used.

This will make dependabot update branches much less useful, since a failing update is almost guaranteed.
In order to remain functional, Dependabot would need to write the corresponding checksum lines to the Gemfile to prevent every CI build from failing.
This would make dependabot the defacto source of the checksums in the Gemfile instead of rubygems.org or .gem files.
This might lead users to disable frozen bundles, disable dependabot, or disable the checksums feature, any of which could be considered major drawbacks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependabot will need updates to work with this feature, and I think that's maybe a good point to make this fully opt-in in the beginning? But I don't understand the point about this leading to users disabling features. As long as Dependabot correctly adds the proper checksums, shouldn't it be fine? That's what Dependabot does for other ecosystems, and it seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I could have made it more clear: "initially, dependabot will not be updated and users may disable the feature with the intention of it being temporary, but it could lead to the feature staying disabled for a long time."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think this is indeed a risk and another reason to let this feature be battle tested for a while in an opt-in only mode first. I don't understand the hurry to force everyone into checksums before we know the full implications.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt-in vs opt-out is interesting. We'll get a lot more useful (if potentially unhappy) feedback from opt-out, but maybe we need an opt-in period first?

Opt in to me would be triggered by running `bundle lock --checksums" and it would add the checksums. We would consider the feature enabled if the lockfile either didn't exist or already had checksums.

My experience using the feature in testing is that most of the time the checksums are not added right away. I think only the frozen-on-ci situation will cause problems with opt-out, and it will hopefully be found at the same time as the person upgrades bundler, so it will be intentional.

I'm open to either really.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be my preferred approach. You get checksums when there's no prior lockfile, or when you explicitly add them.

If a lockfile has no checksums, they can only be added explicitly.

Gemfile.lock changes can be hidden in pull request reviews.
Without a process for checking that the checksums are trustworthy, it's left to every development team to decide on a process.

One solution would be a bundle command that could be run in CI every time the gems are installed that verifies the authenticity of checksums in the Gemfile.lock.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically bundle install in frozen mode in CI would catch it, since there won't be a local gem cache?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant a command for syncing checksums from the remote to ensure you don't have an erroneous checksum generated from a broken gem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmmm, so a command that fixes the issue rather than detecting it, right? Maybe bundle lock without a local gem cache including that broken gem should do that, either by default or under an optional flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, I think it should sync and compare everything. This is the chance to print that "the gem you have installed generated a checksum that is different than what the server is saying."

Bundle lock should just pull all checksums every time then? Lock previously doesn't hit the network unless a resolve is necessary. This would make it always hit the network unless --no-checksums or --local is set.

Copy link
Member Author

@martinemde martinemde Oct 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my explicit concern is this scenario:

  1. Malicious developer sets checksum and upload gem.
  2. Checksum would be invalid if compared with server, but doesn't happen during install.
  3. Malicious gem is successfully installed. (This is a subtle way to bypass security, when a blatant code change might get noticed)

If we had a process that could verify your checksums against the server without altering your bundle, it could run in CI as part of tests and help protect against malicious or bad checksums in lockfiles.

This is extra paranoid though. I think you could just run lock in CI on a frozen bundle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for bundle lock to hit the network (or local cache) to look for checksums.

I'm not sure I understand your concern, can you elaborate this 3 steps? What does "sets checksum" mean? Manually edit a lockfile changing the checksum of a gem? What does "upload gem" mean in this context? Are you implying a situation where a malicious actor has both control of your lockfile, and your remote server? That sounds wild and I don't see how anything can be done in that situation 🤣

Overall, I think bundle install --force in frozen mode should be a strict way to check remote checksums against lockfile checksums, since that bypasses the local cache and should not allow lockfile changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe, maybe my imagination is running a bit wild. With that access you can do whatever you want.

The presence of bundle install --force should provide options for people wanting to use this feature like that.

The more we talk about it, the more clear it is that we can't anticipate everything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that access you can do whatever you want.

Exactly, we shouldn't care about it since at that point there's no much we can do.

If you would like to bypass this behavior, add the `--no-checksums` flag.

Common Bundler commands like `bundle install`, `bundle update`, `bundle lock`, `bundle add` will now automatically record and verify checksums.
Commands that rely on checksums for verification will output a message or warning when checksums are missing or mismatched.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see an argument either way, but it's in my mind a bit of a stretch of what frozen already means (for missing checksums, at least).

text/0011-gem-checksum-verification.md Show resolved Hide resolved
2. run `bundle install`

To ignore checksum security warnings, disable checksum validation with
`bundle config set --local disable_checksum_validation true`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO no, if we want a way to avoid putting checksums in the lock file (which im also not sure we want), it should be under a different flag

When gems come from private gem servers that do not implement the compact index, checksums will not be available.
Bundler will calculate digests from .gem files from sources that don’t supply checksums.

An additional flag for `bundle lock` could be provided that allows reinstalling, and thus calculating the checksum for, gems that don't have a checksum.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the behavior for remote servers will be that there's only one level of verification, i.e., that the checksum of whatever gem that was once installed is recored in the lockfile and should be the same in for future installs.

it will also be checked against what the server reports in the compact index, if that is present (which is what already happens)


### GitHub Dependabot and other automations are at risk of failing.

If a user configures CI to install with frozen bundle, then all dependabot pull requests will fail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why will they fail? will dependabot not add the new lock file checksum in when it runs bundle install?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not, please add a small section with instructions for how automation should be updated to be compatible

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependabot (unfortunately) runs a fixed Bundler version. So until that is updated to a version that knows about checksums, it won't add checksums.

On top of that, Dependabot (also unfortunately) uses Bundler internals a lot, rather than only shelling out to Bundler commands, so it's likely that stuff breaks.

@deivid-rodriguez
Copy link
Member

Overall my main concern here would be how to roll this out.

I tend to think the safest way to battle test this before making it the default would be:

  • Add checksums only to fresh lockfiles, and let existing lockfiles work as they work now for the moment.
  • Additionally, provide an --add-checksums flag to bundle lock for users to opt-in to the feature.

This would be consistent with how I'm approaching rubygems/rubygems#5700, by applying it only to new lockfiles, and having already a bundle lock --add-platform command to opt-in to locking more platforms in an existing lockfile.

Comment on lines +51 to +52
42 gems without checksums.
Use `bundle lock` to add checksums to Gemfile.lock.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote this because it seemed like a nice interface, but now I wonder how I can reliably tell how many gems could have checksums that don't.

Is this useful and we should add it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also brings up the problem of not continually printing this when the source doesn't support checksums. If the source doesn't support it, do we add a special checksum like unavailable=true and then use that as a marker for a gem that doesn't need to be verified? We do support just about any key value pair in the checksum field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first saw this, I understood bundle install by default would only advertise checksums, but not add them, and I thought that was ideal, regardless of showing a number or not. I.e., let people know that this feature is available and tell them how to enable it. And don't change behavior unless a CHECKSUM section is present in the lockfile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we really can't anticipate the problems of this feature, I'm starting to lean this way.

So the plan would be to build into this feature a line that suggests running bundle lock --checksums at the end of every install message until the CHECKSUMS exist in the Gemfile.lock. Until then, we treat the feature as disabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I think a line in bundle install output is not too invasive and properly advertises the feature.

@martinemde
Copy link
Member Author

@deivid-rodriguez for the most part this works so smoothly and invisibly that many people may not notice it.

However, for the frozen bundle situation there are some drawbacks. My instinct is that we should fail in that case, but we hopefully have provided a clear way to avoid the failure in the way of messaging how to add all checksums.

Almost all failure will trigger in CI on the "update bundler version" commit. Then everyone will become aware of how frozen impacts the deployment. Then they will run bundle lock, commit again, and they should be good to go.

Please check my assumptions if you disagree.

@deivid-rodriguez
Copy link
Member

I think not being sure whether we want to even raise if we find mismatches in frozen mode is a symptom that this should be battle tested with users first for a period.

In my opinion, this feature, when enabled, should be strict. But it should not get in the middle unless something bad is happening. I think there are many edge cases that we may only find once this starts being used by people.

Hence my take of letting people experiment with this first, listen to feedback, fix issues, and only then start enforcing checksums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants